Fix Peer Discovery Failing #206
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NetworkPeer
participant Swarm
participant Kademlia
Client->>NetworkPeer: Connect
NetworkPeer->>Swarm: Initialize TCP
Swarm->>Kademlia: Setup Kademlia config
Kademlia-->>Swarm: Configuration complete
Swarm-->>NetworkPeer: Ready to process events
NetworkPeer->>Client: Connection established
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/ciphernode/net/src/network_peer.rs (1)
144-146: Review Gossipsub mesh parameters for optimal network performanceThe mesh parameters in the Gossipsub configuration are set to:
mesh_n(3)mesh_n_low(2)mesh_outbound_min(1)These values determine the mesh's resilience and message propagation efficiency. Ensure these settings align with your network size and desired performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/ciphernode/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
docker-compose.yml(4 hunks)packages/ciphernode/net/Cargo.toml(1 hunks)packages/ciphernode/net/src/network_peer.rs(9 hunks)
🔇 Additional comments (9)
packages/ciphernode/net/src/network_peer.rs (7)
8-11: Imported modules for TCP, Noise, and Yamux look appropriate
The addition of tcp, noise, and yamux modules aligns with the transition from QUIC to TCP, enabling the necessary transports and protocols.
54-58: Swarm initialization with TCP, Noise, and Yamux is correct
The configuration correctly initializes the Swarm to use TCP transport with Noise encryption and Yamux multiplexing. This setup is essential for secure and multiplexed TCP connections.
88-89: Listening address updated to TCP protocol
Updating the listening address to /ip4/0.0.0.0/tcp/{port} reflects the switch to TCP and ensures the node listens on the correct protocol.
98-100: Improved error handling for listen failures
Good practice in logging a warning when the swarm fails to listen on the specified address. This aids in debugging and monitoring.
119-119: Updated event processing call aligns with refactored function
Passing self to process_swarm_event matches the updated function signature and allows for more streamlined access to the swarm and other components.
Line range hint 181-264: Refactored event processing enhances code clarity
Changing process_swarm_event to accept &mut NetworkPeer simplifies access to the swarm and associated components within the function, improving code readability and maintainability.
132-135: Verify the protocol identifiers in Identify and Kademlia configurations
Ensure that the protocol strings "/ipfs/kad/1.0.0" used in the Identify and Kademlia configurations are correct and compatible with the peers in your network. Using consistent protocol identifiers is crucial for proper network communication.
Run the following script to check for consistency of protocol identifiers across the codebase:
✅ Verification successful
The initial search showed matching protocol identifiers, but let's verify there aren't any other variants or definitions that might conflict with these. This will help us ensure complete consistency across the codebase.
Protocol identifiers are consistent in the codebase
The verification shows that the protocol identifier "/ipfs/kad/1.0.0" is used consistently in both the Identify and Kademlia configurations within the same file. No conflicting or inconsistent protocol identifiers were found across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for protocol identifiers in Identify and Kademlia configurations.
# Search for protocol identifiers in Identify configurations
rg 'identify::Config::new\("(.+?)"' packages/
# Search for protocol identifiers in Kademlia configurations
rg 'kad::Config::new\(StreamProtocol::new\("(.+?)"' packages/
Length of output: 348
Script:
#!/bin/bash
# Let's search for any other protocol identifiers in the codebase to ensure consistency
rg -i 'protocol.*"/ipfs/' packages/
# Also search for any configuration or constant definitions that might define these protocols
rg 'const.*PROTOCOL' packages/
rg 'static.*PROTOCOL' packages/
Length of output: 227
packages/ciphernode/net/Cargo.toml (1)
26-28: Included TCP, Noise, and Yamux features for libp2p dependency
Adding "tcp", "noise", and "yamux" to the libp2p features is necessary for enabling TCP transport, secure communication with Noise protocol, and multiplexing with Yamux. This change supports the switch from QUIC to TCP.
docker-compose.yml (1)
15-15: Ports updated to use TCP protocol
Changing the protocol from udp to tcp for services cn1, cn2, cn3, and aggregator ensures that the Docker services expose the correct ports for TCP communication, consistent with the changes in the application code.
Also applies to: 38-38, 60-60, 83-83
| warn!("Failed to redial peer {peer_id}: {e}"); | ||
| } else { | ||
| info!("Redialing peer {peer_id}..."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement backoff strategy for redialing failed peers
Currently, when a dial attempt fails, the code immediately tries to redial the peer. This could lead to rapid repeated dial attempts in case of persistent failures.
Consider adding an exponential backoff or limiting the number of redial attempts to prevent potential resource exhaustion and to comply with polite network behavior.
Apply this diff to implement a simple backoff mechanism:
+use tokio::time::{sleep, Duration};
...
+ let backoff_delay = Duration::from_secs(5);
if let Err(e) = network_peer.swarm.dial(multiaddr.clone()) {
warn!("Failed to redial peer {peer_id}: {e}");
} else {
info!("Redialing peer {peer_id}...");
+ sleep(backoff_delay).await;
}Committable suggestion skipped: line range outside the PR's diff.
Peer Discovery fails through bootstrap despite successful individual connections.
Summary by CodeRabbit
New Features
libp2pdependency with additional features for improved networking capabilities.Bug Fixes
Refactor